Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Load integration from same binary #4541

Merged
merged 26 commits into from
Nov 26, 2024
Merged

Fix: Load integration from same binary #4541

merged 26 commits into from
Nov 26, 2024

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Nov 15, 2024 •
edited
Loading

📜 Description

We use the class name to load integrations, and this could lead to a wrong integration being loaded if sentry is duplicated inside a project.

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 15, 2024 •
edited
Loading

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1240.92 ms 1254.08 ms 13.17 ms
Size 22.30 KiB 748.83 KiB 726.52 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
eed479f 1198.93 ms 1228.12 ms 29.19 ms
9acdca2 1242.45 ms 1249.63 ms 7.18 ms
3b4110a 1228.90 ms 1247.65 ms 18.76 ms
6e342ac 1216.02 ms 1232.88 ms 16.86 ms
b2e7962 1234.70 ms 1241.98 ms 7.28 ms
ca91a5c 1234.53 ms 1249.86 ms 15.33 ms
32c4446 1225.00 ms 1231.29 ms 6.29 ms
ee8b48f 1204.04 ms 1216.70 ms 12.66 ms
25d925a 1232.89 ms 1248.41 ms 15.52 ms
e2abb0d 1235.08 ms 1257.00 ms 21.92 ms

App size

Revision Plain With Sentry Diff
eed479f 20.76 KiB 433.18 KiB 412.42 KiB
9acdca2 22.84 KiB 401.44 KiB 378.59 KiB
3b4110a 21.58 KiB 625.82 KiB 604.24 KiB
6e342ac 20.76 KiB 436.66 KiB 415.90 KiB
b2e7962 21.58 KiB 670.39 KiB 648.81 KiB
ca91a5c 22.84 KiB 403.19 KiB 380.34 KiB
32c4446 22.84 KiB 403.24 KiB 380.39 KiB
ee8b48f 21.58 KiB 418.70 KiB 397.11 KiB
25d925a 21.58 KiB 418.82 KiB 397.24 KiB
e2abb0d 20.76 KiB 434.72 KiB 413.96 KiB

Previous results on branch: fix/load-integrations

Startup times

Revision Plain With Sentry Diff
bbd4c55 1237.27 ms 1253.81 ms 16.55 ms
b1311ae 1244.96 ms 1263.10 ms 18.14 ms
74f63e4 1241.04 ms 1260.04 ms 19.00 ms
d47fa9b 1227.55 ms 1241.30 ms 13.75 ms
f3cf78c 1235.13 ms 1259.86 ms 24.73 ms
44cf191 1230.69 ms 1247.63 ms 16.94 ms

App size

Revision Plain With Sentry Diff
bbd4c55 22.30 KiB 730.95 KiB 708.64 KiB
b1311ae 22.30 KiB 747.68 KiB 725.37 KiB
74f63e4 22.30 KiB 747.71 KiB 725.41 KiB
d47fa9b 22.30 KiB 747.62 KiB 725.31 KiB
f3cf78c 22.30 KiB 747.70 KiB 725.40 KiB
44cf191 22.30 KiB 730.92 KiB 708.62 KiB

@codecov Codecov
Copy link

codecov bot commented Nov 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.940%. Comparing base (37fd7a2) to head (15c4a4e).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
Sources/Sentry/SentrySDK.m 80.000% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4541       +/-   ##
=============================================
- Coverage   91.587%   90.940%   -0.648%     
=============================================
  Files          615       617        +2     
  Lines        69933     70600      +667     
  Branches     25058     25237      +179     
=============================================
+ Hits         64050     64204      +154     
- Misses        5790      6304      +514     
+ Partials        93        92        -1
Files with missing lines Coverage Δ
Sources/Sentry/SentryOptions.m 98.698% <100.000%> (ø)
Sources/Sentry/SentrySDK.m 87.650% <80.000%> (-0.276%) ⬇️

... and 39 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37fd7a2...15c4a4e. Read the comment docs.

---- 🚨 Try these New Features:

armcknight
armcknight reviewed Nov 15, 2024
View reviewed changes
Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing the string-based logic, that has bugged me for a long time. I have a comment on how the method is being declared.

@brustolin brustolin marked this pull request as ready for review November 18, 2024 12:33
philipphofmann
philipphofmann reviewed Nov 18, 2024
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to have a test failing if we change the code in the SentrySDK.m back to the old implementation.

@brustolin
Copy link
Contributor Author

It would be great to have a test failing if we change the code in the SentrySDK.m back to the old implementation.

I considered it, but I’m not sure how to enforce it. It seems like a sketch test.

@philipphofmann
Copy link
Member

It would be great to have a test failing if we change the code in the SentrySDK.m back to the old implementation.

I considered it, but I’m not sure how to enforce it. It seems like a sketch test.

What about using the sample app you created and adding a UI or integration test?

philipphofmann
philipphofmann reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for adding the sample to validate that this doesn't happen again anymore. I added a few comments mostly for improving the readability of the code.

@philipphofmann philipphofmann mentioned this pull request Nov 22, 2024
Open
philipphofmann
philipphofmann approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, when including the more detailed comment.

@brustolin brustolin merged commit 4f43114 into main Nov 26, 2024
@brustolin brustolin deleted the fix/load-integrations branch November 26, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@armcknight armcknight armcknight left review comments

@philipphofmann philipphofmann philipphofmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brustolin @philipphofmann @armcknight @getsentry-bot